-
Notifications
You must be signed in to change notification settings - Fork 6
Pm 2206 master #38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pm 2206 master #38
Conversation
| CREATE SCHEMA IF NOT EXISTS skills; | ||
| CREATE SCHEMA IF NOT EXISTS reviews; | ||
|
|
||
| CREATE EXTENSION IF NOT EXISTS fuzzystrmatch WITH SCHEMA skills; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 design]
The fuzzystrmatch extension is being created in the skills schema. Ensure that this schema is the intended location for this extension, as it might be more conventional to place extensions in the public or pg_catalog schemas unless there's a specific reason for this choice.
|
|
||
| CREATE EXTENSION IF NOT EXISTS fuzzystrmatch WITH SCHEMA skills; | ||
| CREATE EXTENSION IF NOT EXISTS pg_trgm WITH SCHEMA pg_catalog; | ||
| CREATE EXTENSION IF NOT EXISTS pgcrypto WITH SCHEMA reviews; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 design]
The pgcrypto extension is being created in the reviews schema. Verify that this schema is the intended location for this extension, as extensions are typically placed in the public or pg_catalog schemas unless there's a specific reason for this choice.
| CREATE EXTENSION IF NOT EXISTS fuzzystrmatch WITH SCHEMA skills; | ||
| CREATE EXTENSION IF NOT EXISTS pg_trgm WITH SCHEMA pg_catalog; | ||
| CREATE EXTENSION IF NOT EXISTS pgcrypto WITH SCHEMA reviews; | ||
| CREATE EXTENSION IF NOT EXISTS "uuid-ossp" WITH SCHEMA skills; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 design]
The uuid-ossp extension is being created in the skills schema. Confirm that this schema is the intended location for this extension, as it is more common to place extensions in the public or pg_catalog schemas unless there's a specific reason for this choice.
| JOIN pg_namespace ns ON ns.oid = idx.relnamespace | ||
| WHERE idx.relname = 'challenge_phase_challenge_open_end_idx' | ||
| AND ns.nspname = challenge_phase_schema | ||
| AND pg_get_indexdef(idx.oid) LIKE '%("challengeId", "isOpen", "scheduledEndDate", "actualEndDate", name)%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The pg_get_indexdef function is used with a LIKE clause to check index definitions. This approach may be fragile if the index definition changes in the future. Consider using a more robust method to verify index columns, such as querying pg_index and pg_attribute tables directly.
| AND table_name = 'Resource' | ||
| ) THEN | ||
| EXECUTE format( | ||
| 'CREATE VIEW %I.%I AS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
Using SELECT DISTINCT can be costly in terms of performance, especially on large datasets. Consider whether the distinctness is necessary or if it can be ensured by other means, such as unique constraints on the underlying table.
| ELSE | ||
| EXECUTE format( | ||
| 'CREATE VIEW %I.%I AS | ||
| SELECT CAST(NULL AS TEXT) AS "challengeId", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The use of CAST(NULL AS TEXT) in the ELSE branch might lead to confusion or unexpected behavior when querying the view. Consider documenting the intended use case for this scenario or ensuring that downstream consumers of this view handle these NULL values appropriately.
| terms ChallengeTerm[] | ||
| skills ChallengeSkill[] | ||
| auditLogs AuditLog[] | ||
| memberAccesses MemberChallengeAccess[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The addition of memberAccesses as a relation in the Challenge model seems correct. However, ensure that the MemberChallengeAccess model is properly populated and maintained to avoid potential issues with orphaned records or incorrect data associations.
| // MemberChallengeAccess view – member/challenge pairs from resources schema | ||
| ////////////////////////////////////////// | ||
|
|
||
| model MemberChallengeAccess { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The MemberChallengeAccess model lacks auditing fields such as createdAt, createdBy, updatedAt, and updatedBy. Adding these fields would improve traceability and consistency with other models in the schema.
Performance updates to help with PM-2206